@claude review this PR

Claude finished @varex83's task in 3m 53s —— View job

PR Review: feat: add bootnode.rs

Tasks:

• Gather context and understand request
• Read changed files
• Check Go reference implementation (not locally available; reviewed structurally against AGENTS.md and Go patterns)
• Analyze for correctness, safety, and style issues
• Post comprehensive review

Summary

This PR adds bootnode.rs to the p2p crate, implementing relay resolution from HTTP(S) URLs and direct multiaddrs, along with MutablePeer to peer.rs for thread-safe peer state management. The overall structure is good and follows the project patterns, but there are several correctness, naming, and style issues to address before merge.

Findings

[High] resolve_relay exits permanently on first persistent error

Impact: If query_relay_addrs returns a hard failure during startup (e.g., server unreachable), the background task exits and the relay becomes permanently unresolvable for the lifetime of the node — without a restart.

Evidence: crates/p2p/src/bootnode.rs:188-193

Err(e) => {
    tracing::error!(err = %e, url = %raw_url, "Failed resolving relay addresses from URL");
    return;  // ← exits forever
}

query_relay_addrs already exhausts retries, so this is understandable, but continuous relay polling should loop back even after errors. The Go pattern typically uses a continue or loops again with backoff rather than exiting the goroutine.

Recommendation: Instead of return, log the error and continue to the next tokio::select! (sleep + retry next poll interval). Fix this →

[Medium] NotSingleRelayId error variant is dead code

Impact: BootnodeError::NotSingleRelayId is defined (line 87) but never returned anywhere — only a tracing::error! log is emitted at line 204-209. This means callers cannot programmatically handle this condition.

Evidence: crates/p2p/src/bootnode.rs:85-87 (defined), and crates/p2p/src/bootnode.rs:204-209 (only logged)

// Error is defined but this code only logs:
tracing::error!(n = infos.len(), "Failed resolving a single relay ID from addresses");

Recommendation: Either return the error (return Err(BootnodeError::NotSingleRelayId(infos.len()))) or remove the variant from the enum. Fix this →

[Medium] reqwest::Client::new() created on every retry cycle

Impact: A new HTTP client (with connection pool) is created every time query_relay_addrs is called (i.e., every 2 minutes per relay). This throws away the connection pool and TLS session cache on every poll, which defeats the purpose of using reqwest.

Evidence: crates/p2p/src/bootnode.rs:252

let client = reqwest::Client::new();

Recommendation: Accept the client as a parameter, or create it once in resolve_relay and pass it down to each call. Fix this →

[Medium] std::sync::Mutex in async context — violates AGENTS.md

Impact: MutablePeer uses std::sync::Mutex (peer.rs:8, 139). Per AGENTS.md: "Prefer Tokio sync primitives (tokio::sync::*) over std::sync::* when tasks may .await". While the lock is not held across .await points here, the async tasks that call mutable.set() and mutable.peer() violate this project requirement.

Evidence: crates/p2p/src/peer.rs:5-8, 139

use std::sync::{Arc, Mutex};
// ...
inner: Arc<Mutex<MutablePeerInner>>,

Recommendation: Replace with tokio::sync::Mutex and make set, peer, and subscribe async (or use RwLock if you need concurrent reads). Fix this →

[Medium] URL scheme detection is fragile (starts_with("http"))

Impact: Any relay string starting with the characters "http" (e.g., a hypothetical httpXYZ:// scheme) would be treated as an HTTP URL. The correct guards should be starts_with("http://") or starts_with("https://").

Evidence: crates/p2p/src/bootnode.rs:110-111

if relay_addr.starts_with("http") {
    if !relay_addr.starts_with("https") {

Recommendation: Change to starts_with("http://") || starts_with("https://") or parse with url::Url first and check the scheme. Fix this →

[Medium] No IPv6 support in multi_addr_from_enr_str

Impact: record.ip() only returns IPv4 addresses. If a relay ENR only has an IPv6 address (ip6 field), the function will return EnrNoIp and fail silently. Go's multiAddrFromENRStr checks both ip4 and ip6.

Evidence: crates/p2p/src/bootnode.rs:336

let ip = record.ip().ok_or(BootnodeError::EnrNoIp)?;

Recommendation: Check record.ip6() as a fallback when record.ip() is None before returning EnrNoIp.

[Medium] TimeoutQueryingRelayAddresses name misrepresents its use

Impact: The variant is used to signal cancellation, not a timeout. This is confusing to callers and makes it harder to distinguish between actual timeouts and cancellations.

Evidence: crates/p2p/src/bootnode.rs:59, 265-266

#[error("timeout querying relay addresses")]
TimeoutQueryingRelayAddresses,
// ...
if cancel.is_cancelled() {
    return Err(BootnodeError::TimeoutQueryingRelayAddresses);  // ← actually cancelled
}

Recommendation: Rename to Cancelled or QueryCancelled, or add a separate Cancelled variant.

[Low] Redundant clone() in MutablePeer::set

Impact: peer.clone() is called once to store in inner.peer and then called again for each subscriber unnecessarily. Since subscribers take &Peer, the already-stored value can be borrowed.

Evidence: crates/p2p/src/peer.rs:193-194

inner.peer = Some(peer.clone());
inner.subs.iter().for_each(|sub| sub(&peer.clone()));  // ← second clone unnecessary

Recommendation: Use inner.peer.as_deref().unwrap() or just &peer after the first clone(). Fix this →

[Low] Non-200 HTTP response uses wrong error variant

Impact: On a non-200 status code, InvalidRelayUrl is returned (line 284). This is semantically wrong — the URL is valid; the server returned an error. This also means non-200 responses will stop retries (since InvalidRelayUrl is not TimeoutQueryingRelayAddresses), but a clearer error variant would improve observability.

Evidence: crates/p2p/src/bootnode.rs:280-285

tracing::warn!(status_code = resp.status().as_u16(), ...);
return Err(BootnodeError::InvalidRelayUrl);  // ← reuses the wrong variant

[Low] Duplicate logic between addr_info_from_p2p_addr and addr_infos_from_p2p_addrs

Impact: addr_info_from_p2p_addr (private, in bootnode.rs:370-390) duplicates the peer ID extraction and transport addr stripping logic already present in addr_infos_from_p2p_addrs (public, in peer.rs:265-291).

Recommendation: addr_info_from_p2p_addr can be implemented as addr_infos_from_p2p_addrs(&[addr.clone()])?.into_iter().next().ok_or(...).

[Low] P2PConfig::relays type mismatch with new_relays

Impact: P2PConfig::relays is Vec<Multiaddr> (config.rs:60) while new_relays takes &[String]. These should be consistent or there needs to be a documented conversion point.

Evidence: crates/p2p/src/config.rs:60 vs crates/p2p/src/bootnode.rs:106

[Low] No tests for bootnode.rs

Impact: There are no unit tests for multi_addr_from_enr_str, addr_info_from_p2p_addr, or the parsing logic. Per AGENTS.md: "For hashing/serialization parity, generate Go-derived test vectors and hardcode them as Rust fixtures."

Recommendation: Add tests for at minimum multi_addr_from_enr_str with a known ENR string, and addr_info_from_p2p_addr with a known multiaddr string.

Parity Matrix

Tests

Tests were not run as this is a review-only pass.

Open Questions

1. Should new_relays return an error or succeed empty when cancellation fires during the 60-second wait? Currently returns TimeoutResolvingBootnodeEnr on cancellation which is misleading.
2. Is the P2PConfig::relays: Vec<Multiaddr> intentionally different from new_relays's &[String]? If both are used for relay configuration, one of them should be updated for consistency.